BTW, starting standalone without the PASSENGER_DEBUG set,

/Users/pkmiec/.rvm/gems/ree-1.8.7-2012.02/gems/passenger-3.9.1.beta/lib/phusion_passenger/standalone/command.rb:191:in `write_nginx_config_file': undefined local variable or method `passenger_support_files_dir' for #<PhusionPassenger::Standalone::StartCommand:0x10e685ba0> (NameError)

Yep, the Passenger Standalone bug is a different one which I'll fix seperately.

I'm mostly done with the enable/disable process stuff. Could you rebase your patch against HEAD?

Thanks ... back on this again today.

While working on the above feedback, I noticed two problems with master,

In attach(), where the same process could be in the disableWaitlist ... however, the first waiter for some process to call addProcessToList changes the enabled state of that process. This causes further waiters for that same process to fall into the else clause.

In onSessionClose(), you have,

        removeProcessFromList(process, disablingProcesses);
        addProcessToList(process, enabledProcesses);
        removeFromDisableWaitlist(process, DR_SUCCESS, actions);

where I think you meant,

        removeProcessFromList(process, disablingProcesses);
        addProcessToList(process, **disabledProcesses**);
        removeFromDisableWaitlist(process, DR_SUCCESS, actions);

Ah yes, that code in onSessionClose() was a typo. I've fixed that in the bugfixes I committed this weekend.

I don't understand your comment about attach(). You're only supposed to call attach() on a process that isn't yet in the Group, so it couldn't possibly be in the disableWaitlist.

I don't mean that the process being attached is in the disableWaitlist, but rather the disableWaitlist may contain multiple waiters and some of those waiters may point to the same process. So this snippet,

if (process2->enabled == Process::DISABLING && process2->sessions == 0) {
    P_DEBUG("Disabling DISABLING process " << process2->inspect() <<
        "; disable command succeeded immediately");
    removeProcessFromList(process2, disablingProcesses);
    addProcessToList(process2, disabledProcesses);
    postLockActions.push_back(boost::bind(waiter.callback, process2, DR_SUCCESS));
} else {
    newDisableWaitlist.push_back(waiter);
}

should be more like this,

if (process2->sessions == 0) {
  if (process2->enabled == Process::DISABLING) {
    P_DEBUG("Disabling DISABLING process " << process2->inspect() <<
      "; disable command succeeded immediately");
    removeProcessFromList(process2, disablingProcesses);
    addProcessToList(process2, disabledProcesses);
  }
  postLockActions.push_back(boost::bind(waiter.callback, process2, DR_SUCCESS));
} else {
  newDisableWaitlist.push_back(waiter);
}

Thanks. I've merged your patches with a few minor changes.

Everything's looking good so far. If no unexpected problems are found this will be part of beta 2.

How did you like contributing to Passenger 4's codebase? Is all the code clearly understandable, readable, etc?

Awesome!

First, I really appreciate the code base having tests (cxx, ruby, integration). I feel much more confident making changes knowing that all the existing tests still pass. Second, c++ is not my primary language these days and I appreciate your help and patience.

I was able to read through the code and figure out the places I needed to modify fairly quickly. There was enough comments and examples of similar looking code (i.e. spawning a thread, acquiring lock, passing calbacks) for me to piece together what I needed to do.

The parts I had trouble with was mainly related to getting all the details of locking correct (e.g. process is not detached). I still think it would be good idea to invest some time into a macro or a function.